Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2 API (feeds) #1338

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

v2 API (feeds) #1338

wants to merge 1 commit into from

Conversation

powerpaul17
Copy link
Contributor

Feeds part of the v2 API

@codecov-commenter
Copy link

Codecov Report

Merging #1338 (bc280d8) into master (e86a16c) will decrease coverage by 0.24%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1338      +/-   ##
============================================
- Coverage     90.41%   90.16%   -0.25%     
- Complexity      761      765       +4     
============================================
  Files            66       67       +1     
  Lines          2806     2807       +1     
============================================
- Hits           2537     2531       -6     
- Misses          269      276       +7     
Impacted Files Coverage Δ Complexity Δ
lib/Controller/FeedApiV2Controller.php 0.00% <0.00%> (ø) 4.00 <4.00> (?)
lib/Service/FeedServiceV2.php 100.00% <0.00%> (ø) 29.00% <0.00%> (ø%)
lib/Command/Debug/ItemList.php 100.00% <0.00%> (ø) 6.00% <0.00%> (ø%)
lib/Command/Debug/FeedItemList.php 100.00% <0.00%> (ø) 6.00% <0.00%> (ø%)
lib/Command/Debug/FolderItemList.php 100.00% <0.00%> (ø) 8.00% <0.00%> (ø%)
lib/Fetcher/FeedFetcher.php 89.11% <0.00%> (+0.44%) 40.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e86a16c...bc280d8. Read the comment docs.

*/
public function create(
string $url,
string $name = '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be like this

Suggested change
string $name = '',
?string $name = null,

Comment on lines +71 to +72
string $basicAuthUser = null,
string $basicAuthPassword = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will fail with the default because null is not a string

Suggested change
string $basicAuthUser = null,
string $basicAuthPassword = null
?string $basicAuthUser = null,
?string $basicAuthPassword = null

@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Jun 26, 2021
@anoymouserver anoymouserver added 2. developing API Impact API/Backend code and removed stale labels Jun 28, 2021
@powerpaul17
Copy link
Contributor Author

The API documentation lists several error return messages with different status codes but as far as I can see there is no possibility to distinguish between those in the services. Should I try to add this to the service or change/remove it from the docu since it's not possible at the moment?

@SMillerDev
Copy link
Contributor

There are a bunch of errors like "folder name conflict" that don't make sense to error on and have been removed AFAIK. I'd just update the doc for now.

Signed-off-by: Paul Tirk <paultirk@paultirk.com>
@powerpaul17
Copy link
Contributor Author

There are a bunch of errors like "folder name conflict" that don't make sense to error on and have been removed AFAIK. I'd just update the doc for now.

Ok, I'll adapt the API documentation accordingly. We can always add more error codes later if it's possible or desired.

@martinrotter
Copy link

Author of RSS Guard here.

No bad intentions intended: I just want to ask, in what state is APIv2 right now? Is it far from being complete/usable? Any ETA? Why I ask as that I have 1.2 API implemented in RSS Guard (and used by many people) and I want to plan some related things.

Thanks.

@SMillerDev
Copy link
Contributor

Only folders are implemented, the rest is TODO. Seeing how the v2 spec was theorized in 2016 by the last maintainer I wouldn't hold my breath though.

@powerpaul17
Copy link
Contributor Author

I was planning to implement the v2 API but I have very little time (sorry 🙁) and I got stuck because in my opinion the API is not really finished. For example, there is no discussion about what should happen if there are thousands of items on initial sync.. I think an API should really be solid and I don't have much experience with those kind of things so I don't know how to proceed..

@martinrotter
Copy link

As I have quite extensive experience with CONSUMING all kinds of APIs, including "RSS" ones, all I can say is that if you do not want to reinvent the wheel, simply implement Google Reader API.

It is relatively sane API and if implemented well by both server and client, works fast too + it is already implemented in plethora of clients.

https://github.com/bazqux/bazqux-api
https://github.com/theoldreader/api
https://www.inoreader.com/developers/

That said, I really do not think that 1.2 API is bad. It is actually quite good! It only misses some options to be "perfect":

  • do not use two "ID"s for single message (guid, hash), only use ONE ID to identify message
  • add a way to perform FAST two-way sync - method to obtain list of article IDs with some input conditions, add method to get article contents for that list of article IDs

@SMillerDev
Copy link
Contributor

I was planning to implement the v2 API but I have very little time (sorry 🙁)

Don't be sorry, any effort is much appreciated.

simply implement Google Reader API.

Unfortunately that's near impossible within the constraints of being a nextcloud app.

do not use two "ID"s for single message (guid, hash), only use ONE ID to identify message

There is only one ID, it's the nextcloud ID, any other information is purely for convenience.

add a way to perform FAST two-way sync - method to obtain list of article IDs with some input conditions, add method to get article contents for that list of article IDs

Would you mind elaborating on that a bit? This seems like something that might be possible to add without going to a V2 directly.

@martinrotter
Copy link

  1. In 1.2 api, to mark article STARRED, you have to input "guidHash". All other article operation, however, require client to provide "itemId". Those are two distinct IDs. I am not sure if "guidHash" is somehow computed internally in Nextcloud News from "itemId" - anyway each client has to keep two IDs per article now. - It is necessary to eliminate that need and make "starred" methods work with "itemId" too.
  2. 1.2 api from my POV does NOT really miss any major functionality. It is relativelly good, solid, bug-free API. It is tested, mature.
  3. As for fast sync, 1.2 API provides method "Get items" which returns FULL articles (including article "body") given input criteria. So when user/client wants to for example check if he has downloaded all articles per his criteria, he must call "Get items" again and perform comparisons and decisions on the client side. You have method "Get updated items" to aid "incremental" synchronizations but that would not work for all use cases, primarily because it does not for example offer the parameter "getRead" like "Get items" does.

Let's imagine user only wants to efficiently download unread only articles from some feed and feed contains big number of changing unread articles for some reason (trust me, I've seen use-cases you would not imagine in your worst dreams). He calls "Get items" with "getRead=false" and when he wants to check what new unread items there are later, he must call "Get Items" with "getRead=false" again, because method "Get updated items" does not allow to filter with "getRead" parameter (at least docs do not say it can).

Here is where my proposition comes into play.

Design new method "Get items IDs", which will take same input parameters like "Get items" but will be fast and will only produce list of item IDs. Then have one more extra method "Get items from IDs" which will work exactly like "Get items" but will not return items from folder/feed, but from provided IDs list.

That ways, client can call "Get items IDs" to get various "sets" of IDs (list of read IDs, list of unread IDs, list of starred IDs), then compare with local app DB what articles he does not have, what articles moved read=>unread, unread=>read and then download full articles with only IDs he really really wants. This minimizes network bandwith dramatically, particularly for BIG feeds.

I agree that "Get updated items" covers some use-cases, just not all of them. I recommend reading part of that Bazqux API:

https://github.com/bazqux/bazqux-api#the-right-way-to-sync

So current API only really needs to polish and extend a bit, just a couple of parameters, couple of methods. As I see it, we do not really need whole new API as there are really not "design" problems (apart from that doubled article ID thing, which only concerns one method).

Just my opinion, cheers.

@powerpaul17
Copy link
Contributor Author

Martin's suggestions seem like a good idea to me, since I'm not completely convinced of the "finished" state of the v2 API. Would it be acceptable to address those two points (maybe make it an API 1.3 or so) @SMillerDev ? I might look into that for the moment..

@SMillerDev
Copy link
Contributor

Yeah, I'm fine with improving the current API

@powerpaul17 powerpaul17 closed this by deleting the head repository Nov 7, 2022
@powerpaul17 powerpaul17 reopened this Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing API Impact API/Backend code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants